Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add allow_in_sandbox! DSL #17734

Closed
wants to merge 6 commits into from
Closed

Add allow_in_sandbox! DSL #17734

wants to merge 6 commits into from

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jul 14, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR changes the default HOMEBREW_TEMP to be /private/tmp/homebrew on macOS and /tmp/homebrew on Linux. The sandbox permissions are also updated to not allow writing to /private/tmp except for the homebrew subdirectory.

I'm marking this as draft because I am not 100% sure this is the correct move.

Additionally, I'm planning on adding a formula DSL to allow us to reduce the file write sandbox restrictions for formulae that need to write to /private/tmp. I'm thinking about calling this reduce_sandbox! or something similar. That way, this change can be skipped for formulae that need to be updated upstream without blocking us from merging things. Eventually, this might be something that can be enforced so that users can choose to not install any formulae that need a reduced sandbox or something like that.

@Rylan12 Rylan12 added the sandbox Homebrew's use of the macOS Sandbox label Jul 14, 2024
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, good idea!

@Rylan12
Copy link
Member Author

Rylan12 commented Jul 14, 2024

There is some more work to be done here with e.g. adding tests and updating brew style to properly order the reduce_sandbox! DSL, but I would like to get some @Homebrew/core feedback on this before moving forward to make sure the DSL looks good.

Essentially, my vision is that most formulae should be not affected by this change. For the odd one that explicitly writes to /private/tmp (i.e. ignoring TMPDIR), we can add reduce_sandbox! :allow_write_to_temp to indicate that we're okay relaxing the sandbox check for this case. Ideally, this is just used temporarily until upstream is fixed to use the TMPDIR variable, but realistically this won't be fixed everywhere.

In the future, we can add additional things here. For example, once the work discussed in #17703 (comment) is implemented, we might want to deny network activity by default but allow individual formulae to use it with reduce_sandbox! :allow_network

@MikeMcQuaid
Copy link
Member

but I would like to get some @Homebrew/core feedback on this before moving forward to make sure the DSL looks good.

@Rylan12 Often the easiest way to do this is a Homebrew/core PR changing a few formulae so people can see what it's expected to look like (and potentially check out both branches for testing).

we can add reduce_sandbox! :allow_write_to_temp to indicate that we're okay relaxing the sandbox check for this case. Ideally, this is just used temporarily until upstream is fixed to use the TMPDIR variable, but realistically this won't be fixed everywhere.

sounds great 👍🏻

we might want to deny network activity by default but allow individual formulae to use it with reduce_sandbox! :allow_network

Think we already have a separate DSL for allowing sandbox network access that @alebcay worked on?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far, nice work @Rylan12!

# @!method reduced_sandbox
# @return [Array<Symbol>]
# @see .reduce_sandbox
def reduced_sandbox
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def reduced_sandbox
def sandbox_allow

or something is more obvious naming to me than reduced

@alebcay
Copy link
Member

alebcay commented Jul 15, 2024

Think we already have a separate DSL for allowing sandbox network access that @alebcay worked on?

Yes, in #17081. If there are other optional/configurable sandbox settings being added, maybe it makes sense to move the network controls into a new unified DSL.

I haven't been actively promoting the presence of this DSL or adding into formulae, since I was hoping to improve the sandboxing situation on Linux first - it felt weird to me that network calls might be blocked on macOS but not Linux, and I felt like that might introduce another platform-specific thing to debug when troubleshooting build failures.

@MikeMcQuaid
Copy link
Member

I was hoping to improve the sandboxing situation on Linux first

while this is admirable: macOS is our majority platform and the from-source sandboxing for the majority users is primarily "in Homebrew's CI" and "on macOS" so I don't think it's worth blocking progress on Linux support

@MikeMcQuaid
Copy link
Member

Yes, in #17081. If there are other optional/configurable sandbox settings being added, maybe it makes sense to move the network controls into a new unified DSL.

This makes sense to me 👍🏻

@Rylan12
Copy link
Member Author

Rylan12 commented Jul 15, 2024

@alebcay I'm curious what your thinking was for having both allow_network_access! and deny_network_access!?

For the new dsl, I was thinking it would only be allow_in_sandbox!, but I'm not convinced that's the best path.


I just pushed an update renaming the DSL to allow_in_sandbox! and adding :signal and :network options.

This isn't quite ready yet because the DSL needs to be added separately from the actual sandbox changes so it can make it into a tag.

@alebcay do you have a sense of how many formulae would need allow_in_sandbox! :network added once we flip the switch?

I will open some sample homebrew/core PRs later on

@Rylan12 Rylan12 changed the title Use /private/tmp/homebrew as HOMEBREW_TEMP Add allow_in_sandbox! DSL Jul 15, 2024
@alebcay
Copy link
Member

alebcay commented Jul 15, 2024

I'm curious what your thinking was for having both allow_network_access! and deny_network_access!?

Mike provided some justification for the allow function in #17081 (comment):

Relatedly, based on similar DSL addition work like this, it might be nice to have effectively a no-op reverse DSL e.g. disable_offline_phase. This might seem odd but: if this merges, eventually we'll end up flipping the default on formulae to disabling network access unless specifically enabled (and maybe in ~5-10 years: we remove the ability to disable here entirely).

This DSL would also make it easier to have a triage process (perhaps with the environment variable above) for formulae where:

  • neither DSL used: formula hasn't been checked yet
  • enable_* used: formula has been checked and offline works (for at least one method)
  • disable_* used: formula has been checked and offline is broken (for at least one method)

do you have a sense of how many formulae would need allow_in_sandbox! :network added once we flip the switch?

With things as they currently are, any formulae that, for example, rely on a language-specific package manager to pull modules/dependencies (e.g. Rust-based cargo install, Go-based go build) from the internet - those would all fail with the current sandboxing implementation.

I believe that @nandahkrishna is considering/investigating/working on the implementation of a new fetch phase in formulae (see #17703 (comment)) in which such network-requiring operations can run, and then the actual build phase can be kept offline. This would hopefully reduce the amount of cases where network access would need to be enabled.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Aug 16, 2024
@github-actions github-actions bot closed this Aug 23, 2024
@github-actions github-actions bot deleted the tmpdir-sandbox branch August 23, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sandbox Homebrew's use of the macOS Sandbox stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants